Skip to content

Conversation

@tobidae-cb
Copy link
Contributor

@tobidae-cb tobidae-cb commented Aug 6, 2025

🚀 Core Features Added:

  1. Enhanced Snapshot Management System

    • Optimized two-tier snapshot system for better performance
    • Initial snapshots downloaded once and reused across tests
    • Per-test snapshot copies using optimized rsync strategies
    • Remote snapshot source support for cloud-based initial snapshots
  2. Machine Information Collection

    • Added machine metadata collection (type, provider, region, filesystem)
    • Machine info included in benchmark results for better context
  3. Report Backend API Improvements

    • New Go-based REST API backend for serving benchmark data from S3
  4. Frontend Enhancements

    • Updated React frontend to work with new backend API
    • Support for displaying machine information in reports
    • Integration with cloud-stored benchmark results

🔧 Technical Improvements:

  • Optimized rsync strategies for faster snapshot copying
  • Better error handling and logging throughout the system
  • Structured configuration management for import/export operations
  • TTL-based caching for improved performance
  • Docker support for containerized deployment

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Aug 6, 2025

✅ Heimdall Review Status

Requirement Status More Info
Reviews 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 0
Sum 1

@tobidae-cb tobidae-cb changed the title feat: Add download/upload capabilities with cloud storage feat: Add cloud storage integration with S3 upload/download and snapshot management Sep 12, 2025
@tobidae-cb tobidae-cb marked this pull request as ready for review September 12, 2025 18:57
@tobidae-cb tobidae-cb requested a review from meyer9 September 19, 2025 16:22
Copy link
Collaborator

@meyer9 meyer9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks awesome! I love all the new UI elements and how you integrated machine type/etc. Is there a way to make this agnostic to S3?

It looks like we're tying the report generation to S3 now, but users may want to use other cloud storage, or something like Github Actions.

  • export: downloads and uploads to s3 - can be done using aws s3 cp outside of the benchmark tool
  • serving from s3: this is just a static file server pulling from S3 - could just use an existing solution to serve static files

One philosophy I think we should adopt here is: do one thing and do it well. I think adding S3 download/upload/serving is nice to have, but I'm not sure it makes sense to include in the external repo that other teams may use who may not use S3.

The point of serving data from static files instead of a backend is that it makes deployment super simple. All of this work seems compatible with the static file interface, so adding S3 seems slightly unnecessary.

Copy link

@Pjrich1313 Pjrich1313 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Have all POW to contract this work

Copy link
Collaborator

@meyer9 meyer9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Left some more comments, but looks good overall!

I have a few suggestions:

  • remove AWS integration from this repo - this should probably just be in our internal repo
  • remove report backend from this repo - also can be in our internal repo - externally, people can use a static file system

Other than removing AWS dependency, I think the chain copy behavior is a little bit confusing. For ZFS, would I reuse_existing or chain_copy?

@tobidae-cb tobidae-cb force-pushed the tobi/add-aws branch 2 times, most recently from 138892f to a3364ad Compare September 30, 2025 17:43
meyer9
meyer9 previously approved these changes Oct 2, 2025
Copy link
Collaborator

@meyer9 meyer9 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LG! Nothing blocking, just a few small suggestions - feel free to take or leave any of them.

Description: "Runs benchmarks according to the specified config.",
},
{
Name: "import-runs",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

small nit: I think this file can be reverted


## Configuration Format

### New YAML Structure
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we can remove this file. Maybe move some of this explanation into configs/examples/snapshot.yml

@@ -0,0 +1,177 @@
#!/bin/bash
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe rename this to setup-base-snapshot since now it's specific to Base?

@cb-heimdall cb-heimdall dismissed meyer9’s stale review October 2, 2025 20:39

Approved review 3296275630 from meyer9 is now dismissed due to new commit. Re-request for approval.

@tobidae-cb tobidae-cb enabled auto-merge (squash) October 2, 2025 20:41
@tobidae-cb tobidae-cb merged commit 5a8621a into main Oct 2, 2025
10 checks passed
@tobidae-cb tobidae-cb deleted the tobi/add-aws branch October 2, 2025 20:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants